-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add facets to AppliedFilters #43
Conversation
J=1656 TEST=manual
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filterutils
class is getting pretty complicated with all the different things going on. There's grouping, pruning, collecting, etc. I think we really need to clean this file up. But, that's something outside the scope of what you're doing.
filterGroupLabel: string, | ||
filterLabel: string | ||
filterType: 'NLP_FILTER' | 'STATIC_FILTER' | 'FACET', | ||
fieldId: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make things harder if we had the interface as the following:
filterType: ...,
filter: Filter,
count?: number,
groupLabel: string,
label: string
When I see an interface called DisplayableFilter
, I think it makes sense for it to include a Filter
and any additional display metadata for it. I think relating the DisplayableFilter
interface to Filter
has other advantages as well. It's better re-use of code and were we to change Filter
, we would not have to make the same updates to this type as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, we might consider adding a type
param to the Filter
interface. That seems like a good fit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
part of the main purpose of having DisplayableFilter
is so it can unify the structure of different filters (AppliedQueryFilter, Filter, and DisplayableFacets - doesn't use Filter), Otherwise, it would be more tricky and messy to perform comparison and/or extract information from different applied filter types just pulling from the actually nvm, that makes sense, I didn't interpret your comment right initially. Updated!filter
field.
sample-app/src/utils/filterutils.tsx
Outdated
@@ -1,20 +1,28 @@ | |||
import { AppliedQueryFilter, CombinedFilter, Filter } from '@yext/answers-core'; | |||
import { AppliedQueryFilter, CombinedFilter, Filter, DisplayableFacet, NearFilterValue } from '@yext/answers-core'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an idea for how we could re-organize this file. I think, conceptually, it would be simpler if we had a file that only operated on the DisplayableFilter
level of abstraction. That file would contain the pruning and grouping.
We'd have another file that was meant to convert the filters, facets, and applied query filters into DisplayableFilter
s.
I think this makes for a better separation of concerns. We don't have one file spanning multiple levels of abstraction. Right now this file has broadly two responsibilities: generating a list of DisplayableFilters
and then modifying it. We should delegate the first responsibility to methods in another file. Those methods then provide the initial list that methods in this class will pair down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made an update to reorganize filterutils
file:
filterutils
file: contains only standard Filter functions (i.e. isDuplicateFilter, isCombinedFilter, flattenFilters)displayablefilterutils
file: functions that convert filters, facets, and applied query filters into DisplayableFilters.appliedfilterurils
file: pruning, grouping, etc. that is use for the purpose of spitting out an grouped filter array for appliedfilter
J=1656
TEST=manual